fix(sdk): emit RFC 7518 raw ECDSA signatures in JWS (DSPX-3397) - #987
Draft
dmihalcik-virtru wants to merge 2 commits into
Draft
fix(sdk): emit RFC 7518 raw ECDSA signatures in JWS (DSPX-3397)#987dmihalcik-virtru wants to merge 2 commits into
dmihalcik-virtru wants to merge 2 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dmihalcik-virtru
force-pushed
the
dspx-3397-1-jws-signatures
branch
from
August 10, 2026 17:47
756b4b3 to
b220a91
Compare
cryptoService.sign() returns DER-encoded ECDSA signatures, but JWS requires
raw IEEE P1363 (R || S) per RFC 7518 section 3.4. Both JWS emitters -- the KAS
rewrap request token (tdf3/src/crypto/jwt.ts) and the DPoP proof signer
(src/auth/dpop.ts) -- were shipping DER, so any EC-keyed token was rejected by
conformant verifiers (Keycloak, panva-jose). verifyJwt had the mirror bug: it
fed a raw JWS signature to a verifier expecting DER.
Also fixes reqSignature defaulting to RS256 for the rewrap request token.
WebCrypto rejects signing an EC private key with RSA params ("Unable to use
this key to sign"), so an EC dpop key could not produce a rewrap token at all;
the alg is now derived from the key's algorithm.
Supporting changes:
- export ieeeP1363ToDer / derToIeeeP1363 for these callers
- add isAsymmetricSigningAlgorithm() so the JWS `alg` header is validated
rather than blind-cast to the narrower set CryptoService can actually sign
with (the JWS alg space includes PS256/EdDSA, which we do not support)
The round-trip this creates (sign encodes to DER, caller decodes back to raw)
is tracked for removal in DSPX-3634.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
dspx-3397-1-jws-signatures
branch
from
August 11, 2026 17:54
b220a91 to
0f1935a
Compare
…s (DSPX-3397) Follow-up cleanup on the raw-ECDSA JWS fix. No behavior change. Comments: drop the over-explanatory RFC 7518 asides left behind in src/auth/dpop.ts, tdf3/src/crypto/jwt.ts, and the CryptoService sign/verify docs. The encoding contract is stated once where it belongs rather than repeated at every call site. tests/mocha/helpers/jws-keys.ts: the helper hand-rolled utilities the SDK already ships. Replace them with existing functions -- - derToPem / the DER -> PEM -> re-import dance -> wrapPublicKey, wrapPrivateKey (crypto/core/keys.ts) plus exportPublicKeyPem for the PEM the jose-side verification needs - the inline RSASSA-PKCS1-v1_5 keygen -> generateSigningKeyPair() (crypto/core/rsa.ts), which uses identical parameters - encodeBase64url -> deleted, it had no callers - decodeBase64url -> jose's base64url.decode - local NamedCurve / EcdsaAlg -> ECCurve / EcSigningAlgorithm - type PemKeyPair -> TestKeyPair, the old name collided with an unrelated PemKeyPair in crypto-utils.ts ECDSA keygen stays on raw WebCrypto because generateECKeyPair produces ECDH deriveBits keys that cannot sign. tests/mocha/unit/assertions.spec.ts: was duplicating that helper with two inline ECDSA P-256 keygens and four hand-written branded key literals. Use the shared fixture and exportPublicKeyJwk instead, removing four `as any` casts. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 1/6, split out of #939. Base:
main.What
cryptoService.sign()returned DER-encoded ECDSA signatures, but JWS requires raw IEEE P1363 (R || S) per RFC 7518 §3.4. Every JWS emitter was shipping DER:tdf3/src/crypto/jwt.ts— the KAS rewrap request token, and the assertion bindingsrc/auth/dpop.ts— the DPoP proofso any EC-keyed token was rejected by conformant verifiers. This is the "Invalid token signature" / "unable to verify request token" failure seen against Keycloak.
verifyJwthad the mirror bug: it fed a raw JWS signature to a verifier that expects DER.Separately,
reqSignaturedefaulted toRS256for the rewrap request token. WebCrypto rejects signing an EC private key with RSA params (Unable to use this key to sign), so an EC dpop key could not produce a rewrap token at all. The alg is now derived from the key's algorithm.Why it wasn't caught
The SDK's own sign/verify pair was symmetric — both sides spoke DER — so it round-tripped internally even when the wire format was wrong. The mock test server only
decodeJwts the rewrap token and never checks the signature. Both passed while the real platform failed.The new tests verify against
jose.jwtVerify, an independent RFC-conformant verifier (the same library Keycloak uses), which is what closes that gap.Approach: remove DER, don't transcode
An earlier revision of this PR converted DER→raw at each call site, leaving
sign()emitting DER and every caller immediately undoing it. That round-trip is gone:sign()returns raw IEEE P1363 forES*andverify()takes raw, so the bytes go from WebCrypto straight to the wire unchanged. There is no ASN.1 DER code left in the SDK.No compatibility shim is included, deliberately:
binding.signature; the DPoP proof and rewrap token are ephemeral. Producing an EC-signed assertion requires hand-constructing anAssertionKey— the CLI has no flag for it andAssertionKeyAlgisES256 | RS256 | HS256with no EC key generation path — and a GitHub-wide search found no callers.CryptoServiceplugins already speak raw. The virtru FIPS implementation (libraries/fips-web-crypto) emitssize*2fixed-width bytes and its verifier hard-rejects anything else ("ECDSA signature must be %d bytes"). A DER-tolerant design would have broken it.This supersedes DSPX-3634 (formerly stack 8/8, #996), which is now closed as subsumed.
Changes
tdf3/src/crypto/core/signing.ts: deleteieeeP1363ToDer/derToIeeeP1363and the per-curve component-width table;sign()/verify()now pass WebCrypto's bytes throughtdf3/src/crypto/jwt.ts,src/auth/dpop.ts: no conversion at the call sitestdf3/src/tdf.ts:signingAlgForKeyAlgorithm()picks the rewrap token alg from the dpop keytdf3/src/crypto/declarations.ts: deriveAsymmetricSigningAlgorithmfrom a runtimeas constlist so the type and the guard can't drift; addisAsymmetricSigningAlgorithm()so the JWSalgheader is validated rather than blind-cast (the JWS alg space includesPS256/EdDSA, which we cannot sign with); document the ECDSA encoding contract onCryptoService.sign/verify, which previously said nothing either waytdf3/src/assertions.ts: guard the uncheckederror.messagederef so a non-Errorthrow doesn't render asundefinedPublic API note
CryptoService.sign()is public and injectable viaclientConfig.cryptoService. Its ECDSA output encoding changes from DER to raw P1363, and that contract is now written down in the interface docs. A custom implementation that returns DER will produce tokens conformant verifiers reject.Tests
tests/mocha/dpop-proof.spec.ts— DPoP proofs (ES256/384/512, RS256) verified withjose.jwtVerify, plus tamper and wrong-key casestests/mocha/reqsignature-jws.spec.ts— rewrap request token, same treatmenttests/mocha/helpers/jws-keys.ts— shared WebCrypto→PEM→SDK key fixtures for both suitestests/mocha/encrypt-decrypt.spec.ts— end-to-end decrypt with an EC dpop keytests/mocha/unit/crypto/crypto-service.spec.ts— assertssign()returns exactly 64/96/132 bytes per curve, over several draws. Exact width is the discriminator: DER for the same curves is 69–72 / 101–104 / 138–141 bytes, and roughly 1 raw signature in 256 happens to start with0x30, so the DER tag byte alone proves nothing. This replaces the old test that assertedder[0] === 0x30— which pinned the bug in place.tests/mocha/unit/assertions.spec.ts— pins the persistedbinding.signatureat 64 bytes for ES256, since that is the one signature this SDK writes to durable storageTest-gap follow-ups
The same symmetric-round-trip blind spot exists downstream. Filed: DSPX-4331 (js-lib-monorepo: cross-verify FIPS ECDSA against WebCrypto), DSPX-4332 (xtest: ES256 assertion fixture + SDK-independent manifest width check), DSPX-4333 (xtest: ES256 DPoP tests silently skip for the js SDK).
How to test
cd lib && npm test